-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add price component #4
Conversation
react/Price.js
Outdated
|
||
return ( | ||
<div className='tc b fabriga'> | ||
{ listPriceElement } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
geralmente não separamos que tá dentro do render em constantes. Não é legível. Coloca tudo no render e quando for necessário, faz um novo componente para ser reusado e ficar mais modularizado.
react/Price.js
Outdated
* @property {number} [installmentPrice] - Single installment price | ||
*/ | ||
static propTypes = { | ||
sellingPrice: PropTypes.number.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propTypes geralmente ficam depois da classe e não precisam ser estáticas 😬
Pode ser assim:
Price.propTypes = { sellingPrice: PropTypes.number.isRequired, ... }
react/Price.js
Outdated
<FormattedMessage id='pricing.from'/> | ||
</div> | ||
<div className='dib strike ph2'> | ||
{ formatNumber(listPrice, currencyOptions) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
faltando indentação. Vcs usam lint e/ou prettier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ainda não estamos utilizando nenhum. Qual deveríamos usar e como configurar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tem um comando no CLI da VTEX para gerar uma configuração de lint vtex eslint local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adicionei os dois
react/Price.js
Outdated
@@ -0,0 +1,96 @@ | |||
import React, { Component } from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { FormattedMessage, injectIntl } from 'react-intl'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importa também o intlShape
para fazer a validação da prop intl
.
intl: intlShape.isRequired
react/Price.js
Outdated
<FormattedMessage id='pricing.from'/> | ||
</div> | ||
<div className='dib strike ph2'> | ||
{ formatNumber(listPrice, currencyOptions) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tem um comando no CLI da VTEX para gerar uma configuração de lint vtex eslint local
pages/pages.json
Outdated
"component": "Price", | ||
"props": { | ||
"showListPrice": true, | ||
"listPrice": 120, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunojdo comentei com @estacioneto sobre esse código hard-coded. A gente costuma não aceitar nada assim aqui, mas também sei que o projeto está iniciando agora. O que acha? Sugeri deixar on hold enquanto a lógica não é implementada
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mas você que decide 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Além disso, este trecho é apenas para mostrar o componente funcionando como extension point na tela principal. Posso remover se preferir, @brunojdo . Removo esta parte e mostro pessoalmente o funcionamento
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concordo com @analuizamtg tem que tirar. Quando chegar eu vejo o funcionamento.
react/index.js
Outdated
@@ -3,6 +3,8 @@ import PropTypes from 'prop-types' | |||
import {ExtensionPoint, Link, Helmet} from 'render' | |||
import {FormattedMessage, FormattedHTMLMessage, injectIntl, intlShape} from 'react-intl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esse componente FormattedHTMLMessage
está sendo usado em algum lugar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não achei também. Dá uma checada @estacioneto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não. Removi. Apesar de que foi importado automaticamente pelo boilerplate do projeto.
pages/pages.json
Outdated
"component": "Price", | ||
"props": { | ||
"showListPrice": true, | ||
"listPrice": 120, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concordo com @analuizamtg tem que tirar. Quando chegar eu vejo o funcionamento.
react/index.js
Outdated
@@ -3,6 +3,8 @@ import PropTypes from 'prop-types' | |||
import {ExtensionPoint, Link, Helmet} from 'render' | |||
import {FormattedMessage, FormattedHTMLMessage, injectIntl, intlShape} from 'react-intl' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não achei também. Dá uma checada @estacioneto
react/Price.js
Outdated
intl: intlShape.isRequired, | ||
} | ||
|
||
Price.schema = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quem tem schema é o Product Summary. Retirar
No description provided.